system/popen: Avoid copying FILE by nightt5879 · Pull Request #3511 · apache/nuttx-apps

nightt5879 · 2026-05-29T07:28:48Z

Summary

system/popen was embedding a copied FILE object in its private tracking container and copying it back in pclose(). That depends on libc FILE layout details.

This PR makes popen() return the actual stream from fdopen() and keeps a small private FILE * -> pid tracking list so pclose() can find the spawned shell process without copying FILE.

Scope:

Does not change supported popen() modes.
Does not change the spawn/file-action setup.
Keeps pclose() responsible for closing the stream and waiting for the shell pid.
Returns EINVAL if pclose() cannot find the stream in the private popen list.

Impact

popen()/pclose() no longer rely on libc FILE internals.

New feature: NO
User adaptation required: NO
Build process change: NO
Hardware/architecture/board change: NO
Documentation update required: NO
Security impact: NO
Compatibility impact: NO intended compatibility impact

Testing

Host:

Windows with WSL Ubuntu 24.04
Target: sim:nsh

Checks:

git diff --check: pass
checkpatch.sh -g HEAD~1..HEAD: pass
sim:nsh build with CONFIG_SYSTEM_POPEN=y and CONFIG_EXAMPLES_POPEN=y: pass
- confirmed CC: popen.c
- result: SIM elf with dynamic libs archive in nuttx.tgz
printf 'popen\npoweroff\n' | timeout 30s ./nuttx: pass
- confirmed popen("help") output is read
- confirmed execution reaches pclose()

acassis · 2026-05-29T13:21:43Z

@nightt5879 it would be nice to have some test coverage to test it too.

nightt5879 · 2026-05-29T13:37:47Z

Thanks, I added a small testing/libc/popen test in commit 23a698f. @acassis

The test opens two popen(..., "r") streams, verifies the output read from both streams, and closes the first stream before the second. That covers reading from the returned popen() stream and also exercises the private stream-to-pid lookup/removal path added by this PR when the stream is not the first entry in the tracking list.

I verified the added test with:

checkpatch.sh -g HEAD~1..HEAD
sim:nsh build with CONFIG_SYSTEM_POPEN=y and CONFIG_TESTING_POPEN_TEST=y
printf 'popen_test\npoweroff\n' | timeout 30s ./nuttx

The simulator run prints popen_test: successful.

acassis · 2026-05-29T13:46:47Z

Thanks, I added a small testing/libc/popen test in commit 23a698f. @acassis

The test opens two popen(..., "r") streams, verifies the output read from both streams, and closes the first stream before the second. That covers reading from the returned popen() stream and also exercises the private stream-to-pid lookup/removal path added by this PR when the stream is not the first entry in the tracking list.

I verified the added test with:
* `checkpatch.sh -g HEAD~1..HEAD`

* `sim:nsh` build with `CONFIG_SYSTEM_POPEN=y` and `CONFIG_TESTING_POPEN_TEST=y`

* `printf 'popen_test\npoweroff\n' | timeout 30s ./nuttx`
The simulator run prints popen_test: successful.

My bad, I looked only the first commit. So, everything is fine!

nightt5879 · 2026-05-29T13:48:20Z

Thanks, I added a small testing/libc/popen test in commit 23a698f. @acassis
The test opens two popen(..., "r") streams, verifies the output read from both streams, and closes the first stream before the second. That covers reading from the returned popen() stream and also exercises the private stream-to-pid lookup/removal path added by this PR when the stream is not the first entry in the tracking list.
I verified the added test with:
* `checkpatch.sh -g HEAD~1..HEAD`

* `sim:nsh` build with `CONFIG_SYSTEM_POPEN=y` and `CONFIG_TESTING_POPEN_TEST=y`

* `printf 'popen_test\npoweroff\n' | timeout 30s ./nuttx`
The simulator run prints popen_test: successful.
My bad, I looked only the first commit. So, everything is fine!

Just to clarify: I added the additional commit after seeing your suggestion.
Thanks for reviewing it again!

nightt5879 · 2026-05-31T05:05:17Z

@xiaoxiang781216 Thanks, I addressed the latest review comments in commit 6a16a4d.

The follow-up changes:

changed the invalid-mode path to jump to errout_with_container, since no pipe fd exists there;
avoided closing the stream fd twice after fopencookie() has taken ownership;
removed the redundant NULL check before free();
moved fd close, waitpid(), and cookie free into popen_file_close(), so pclose() now calls fclose() directly.

One caveat I noticed while moving waitpid() into the fopencookie close callback: fclose() treats any non-OK close callback return as an EOF/error result. Because of that, popen_file_close() now returns OK after close() and waitpid() themselves succeed, and reports only close()/waitpid() failures as errors. Otherwise a normal nonzero shell termination status could be converted into an fclose() failure.

I re-tested with:

checkpatch.sh -f apps/system/popen/popen.c
sim:nsh build with CONFIG_SYSTEM_POPEN=y and CONFIG_TESTING_POPEN_TEST=y
popen_test, which prints successful

xiaoxiang781216 · 2026-06-02T15:36:05Z

@nightt5879 please rebase your patch to fix the conflict, thanks.

Use fopencookie() to attach the popen fd and shell pid to the returned FILE stream instead of copying FILE into the popen container. Keep the upstream dpopen()/dpclose() implementation as the process and descriptor backend, and make pclose() close the cookie-backed stream directly. Fixes apache#2937. Signed-off-by: Nightt <87569709+nightt5879@users.noreply.github.com>

nightt5879 · 2026-06-03T01:57:10Z

@xiaoxiang781216 Rebased onto the latest master and force-pushed the branch.

The conflict was resolved by keeping the upstream dpopen()/dpclose() implementation and the new testing/libc/popen test, then reapplying only the fopencookie() change in popen.c to avoid copying FILE.

I re-tested with:

git diff --check upstream/master..HEAD
checkpatch.sh -f ../apps/system/popen/popen.c
sim:nsh build with CONFIG_SYSTEM_POPEN=y and CONFIG_TESTING_POPEN=y
popen_test, which reports 0 failed

xiaoxiang781216 · 2026-06-03T03:27:22Z

@xiaoxiang781216 Rebased onto the latest master and force-pushed the branch.

The conflict was resolved by keeping the upstream dpopen()/dpclose() implementation and the new testing/libc/popen test, then reapplying only the fopencookie() change in popen.c to avoid copying FILE.

I re-tested with:

git diff --check upstream/master..HEAD

checkpatch.sh -f ../apps/system/popen/popen.c

sim:nsh build with CONFIG_SYSTEM_POPEN=y and CONFIG_TESTING_POPEN=y

popen_test, which reports 0 failed

Thanks, the new version is much simple now.

github-actions Bot added Size: M Area: System labels May 29, 2026

nightt5879 marked this pull request as ready for review May 29, 2026 07:34

acassis previously approved these changes May 29, 2026

View reviewed changes

nightt5879 dismissed acassis’s stale review via 23a698f May 29, 2026 13:37

xiaoxiang781216 reviewed May 29, 2026

View reviewed changes

Comment thread system/popen/popen.c Outdated

nightt5879 force-pushed the nightt5879/fix-popen-file-copy-2937 branch from 23a698f to 9549d3b Compare May 30, 2026 05:07

xiaoxiang781216 reviewed May 31, 2026

View reviewed changes

Comment thread system/popen/popen.c Outdated

Comment thread system/popen/popen.c Outdated

Comment thread system/popen/popen.c Outdated

Comment thread system/popen/popen.c Outdated

Comment thread system/popen/popen.c Outdated

xiaoxiang781216 reviewed Jun 1, 2026

View reviewed changes

Comment thread system/popen/popen.c Outdated

Comment thread system/popen/popen.c Outdated

nightt5879 force-pushed the nightt5879/fix-popen-file-copy-2937 branch from 433405a to 04ead82 Compare June 3, 2026 01:56

xiaoxiang781216 approved these changes Jun 3, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

system/popen: Avoid copying FILE#3511

system/popen: Avoid copying FILE#3511
nightt5879 wants to merge 1 commit into
apache:masterfrom
nightt5879:nightt5879/fix-popen-file-copy-2937

nightt5879 commented May 29, 2026

Uh oh!

acassis commented May 29, 2026

Uh oh!

nightt5879 commented May 29, 2026 •

edited

Loading

Uh oh!

acassis commented May 29, 2026

Uh oh!

nightt5879 commented May 29, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nightt5879 commented May 31, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

xiaoxiang781216 commented Jun 2, 2026

Uh oh!

nightt5879 commented Jun 3, 2026

Uh oh!

xiaoxiang781216 commented Jun 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

nightt5879 commented May 29, 2026

Summary

Impact

Testing

Uh oh!

acassis commented May 29, 2026

Uh oh!

nightt5879 commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acassis commented May 29, 2026

Uh oh!

nightt5879 commented May 29, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nightt5879 commented May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiaoxiang781216 commented Jun 2, 2026

Uh oh!

nightt5879 commented Jun 3, 2026

Uh oh!

xiaoxiang781216 commented Jun 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nightt5879 commented May 29, 2026 •

edited

Loading

nightt5879 commented May 31, 2026 •

edited

Loading